Add FLOP estimates for operator assembly - #2018
Conversation
jeremylt
left a comment
There was a problem hiding this comment.
not sure how I feel about having these tests do two different things instead of one, but their descriptions should be updated
|
it should me possible to estimate at points too |
Signed-off-by: alvaroborras <13698600+alvaroborras@users.noreply.github.com>
c2ae795 to
9f46b6c
Compare
|
Thanks for the comments, I added some changes around point estimates that I was working on. |
|
See failing CI jobs - release notes need to be updated, and there's as style fix |
Signed-off-by: alvaroborras <13698600+alvaroborras@users.noreply.github.com>
|
I’ve updated the test descriptions and comments, added the required release-note entry, and applied the style fix. For AtPoints assembly estimates, CPU backends now count the actual total number of points, while GPU backends keep the max points in element count. I've also added a regression test to cover both cases. Thank you for the feedback. |
Signed-off-by: alvaroborras <13698600+alvaroborras@users.noreply.github.com>
|
Looking really close, just a couple more small things I noticed |
|
Thanks for the feedback. I’ve updated the release note ordering, reused the existing point count helper, and added the missing documentation. |
jeremylt
left a comment
There was a problem hiding this comment.
two minor things, but otherwise looks ready
Document that QFunction FLOP estimates depend on rebuild status without modifying internal state. Reuse the point-count temporary across host and device paths, follow interface error-handling conventions, and keep the CHANGELOG chronological. Signed-off-by: alvaroborras <13698600+alvaroborras@users.noreply.github.com>
c581f18 to
76da6ea
Compare
| if (is_at_points) { | ||
| CeedInt num_elem, num_points; | ||
| CeedMemType mem_type; | ||
| CeedElemRestriction rstr_points; | ||
|
|
||
| CeedCall(CeedOperatorAtPointsGetPoints(op, &rstr_points, NULL)); | ||
| CeedCall(CeedOperatorGetNumElements(op, &num_elem)); | ||
| CeedCall(CeedGetPreferredMemType(CeedOperatorReturnCeed(op), &mem_type)); | ||
| if (mem_type == CEED_MEM_DEVICE) { | ||
| // Device backends pad every element to the maximum number of points. | ||
| CeedCall(CeedElemRestrictionGetMaxPointsInElement(rstr_points, &num_points)); | ||
| num_qpts_total = (CeedSize)num_elem * num_points; | ||
| } else { | ||
| CeedCall(CeedElemRestrictionGetNumPoints(rstr_points, &num_points)); | ||
| num_qpts_total = num_points; |
There was a problem hiding this comment.
| if (is_at_points) { | |
| CeedInt num_elem, num_points; | |
| CeedMemType mem_type; | |
| CeedElemRestriction rstr_points; | |
| CeedCall(CeedOperatorAtPointsGetPoints(op, &rstr_points, NULL)); | |
| CeedCall(CeedOperatorGetNumElements(op, &num_elem)); | |
| CeedCall(CeedGetPreferredMemType(CeedOperatorReturnCeed(op), &mem_type)); | |
| if (mem_type == CEED_MEM_DEVICE) { | |
| // Device backends pad every element to the maximum number of points. | |
| CeedCall(CeedElemRestrictionGetMaxPointsInElement(rstr_points, &num_points)); | |
| num_qpts_total = (CeedSize)num_elem * num_points; | |
| } else { | |
| CeedCall(CeedElemRestrictionGetNumPoints(rstr_points, &num_points)); | |
| num_qpts_total = num_points; | |
| if (is_at_points) { | |
| CeedMemType mem_type; | |
| CeedElemRestriction rstr_points; | |
| CeedCall(CeedOperatorAtPointsGetPoints(op, &rstr_points, NULL)); | |
| CeedCall(CeedGetPreferredMemType(CeedOperatorReturnCeed(op), &mem_type)); | |
| if (mem_type == CEED_MEM_DEVICE) { | |
| CeedInt num_elem, num_points; | |
| // Device backends pad every element to the maximum number of points. | |
| CeedCall(CeedOperatorGetNumElements(op, &num_elem)); | |
| CeedCall(CeedElemRestrictionGetMaxPointsInElement(rstr_points, &num_points)); | |
| num_qpts_total = (CeedSize)num_elem * num_points; | |
| } else { | |
| CeedCall(CeedElemRestrictionGetNumPoints(rstr_points, &num_qpts_total)); |
Ok, this is what I was trying to say. Generally, variables should only have the scope they really need and nothing bigger
|
@jeremylt said:
Just a note that this will distort overall flop rates, even for surrounding events. One could find that switching to a GPU causes Gordon Bell (with misguided emphasis on reporting high flop/s rates) has rewarded teams for using naive (non-tensor) kernels for spectral element simulation because the end-to-end flop/s rate is higher, even when everything is slower and the flop/s rate achieved in the element kernels are not even faster. They might as well have plopped in a DGEMM that goes completely unused. So we (the libCEED maintainers and community) should at least be aware of the implications, though I can't think of an obvious place to document the interpretation effects of this specifically. (In principle one could distinguish flops performed from useful flops, but that gets messy and subjective, and hard to interpret hardware limits.) |
|
Yeah, that's a good point. Same thing applies to our element block padding, and I think (but don't recall at the moment) that we similarly count flops done and not flops needed? |
|
Yeah, lots of such examples. Meanwhile, GPU vendors count flops not performed with their "sparse" matrix primitives. (Basically, they are saying if you were to pad with zeros and then perform the naive dense operation, this is how many flops would have been done.) |
Purpose:
Add FLOP estimates for full, diagonal, and point-block diagonal operator assembly. The estimates account for stale QFunction assembly data and report AtPoints assembly as unsupported.
Closes: #1983
LLM/GenAI Disclosure:
Codex was used to brainstorm and review the changes, but the code was written by hand.
By submitting this PR, the author certifies to its contents as described by the Developer's Certificate of Origin.
Please follow the Contributing Guidelines for all PRs.